Improve how `QueryCache`s and `QueryState`s are stored by nnethercote · Pull Request #152835 · rust-lang/rust

nnethercote · 2026-02-19T03:38:04Z

QueryVTable has two fields query_state and query_cache that are byte offsets of fields in other structs. They are used in unsafe combination with byte_add and cast to access said fields. I don't like this at all and this PR replaces them with something sensible.

r? @Zalathar

The tuple used by `mk_query_stack_frame_extra` currently uses `QueryVTable`. The next commit will make `QueryVTable` no longer impl `DynSync`, which will prevent it from being used in the tuple. So this commit changes the tuple to use just the three necessary fields from `QueryVTable` instead of the whole thing.

`QuerySystem` has these fields: ``` pub states: QueryStates<'tcx>, pub caches: QueryCaches<'tcx>, pub query_vtables: PerQueryVTables<'tcx>, ``` Each one has an entry for each query. Some methods that take a query-specific `QueryVTable` (via a `SemiDynamicQueryDispatcher` wrapper) need to access the corresponding query-specific states and/or caches. So `QueryVTable` stores the *byte offset* to the relevant fields within `states` and `caches`, and uses that to (with `unsafe`) access the fields. This is bizarre, and I hope it made sense in the past when the code was structured differently. This commit moves `QueryState` and `QueryCache` inside `QueryVTable`. As a result, the query-specific states and/or caches are directly accessible, and no unsafe offset computations are required. Much simpler and more normal. Also, `QueryCaches` and `QueryStates` are no longer needed, which makes `define_callbacks!` a bit simpler. `QueryVTable` no longer impls `DynSync`, which required the change in the preceding commit.

Currently type variables that impl `QueryCache` are called either `C` or `Cache`. I find the former clearer because single char type variables names are very common and longer type variable names are easy to mistake for type or trait names -- e.g. I find the trait bound `C: QueryCache` much easier and faster to read than `Cache: QueryCache`.

rustbot · 2026-02-19T03:38:09Z

Zalathar is not on the review rotation at the moment.
They may take a while to respond.

Zalathar · 2026-02-19T03:50:57Z

@bors try @rust-timer queue

Improve how `QueryCache`s and `QueryState`s are stored

Zalathar · 2026-02-19T04:01:26Z

Putting the state and cache directly in the vtable is not a possibility I had considered.

It stretches the concept of “vtable” a bit, and it gives up on being able to treat the vtable as mere metadata, but it does have advantages for actually looking up the state/cache given a vtable reference.

Genuinely unsure how to feel about this overall. It doesn’t jump out to me as something obviously-good or obviously-bad; the tradeoffs are more subtle.

nnethercote · 2026-02-19T04:33:36Z

I agree about stretching the meaning of "vtable".

Beyond that I think it's a clear win. The offset code is ridiculous, it screams "something went wrong here".

More generally, it makes more sense to have a struct containing (A, B, C) x 400 than having (A x 400, B x 400, C x 400) when A and B and C are closely related and used together.

Zalathar · 2026-02-19T05:10:03Z

This PR also touches on some areas that I haven’t been able to work on because they’re still waiting for #152747 to be merged, so having this PR also interfere with that is starting to feel overwhelming.

rust-bors · 2026-02-19T06:02:31Z

☀️ Try build successful (CI)
Build commit: 6efacd9 (6efacd91cd7cd06fc2c44d60f80077051f2fca7d, parent: e0cb264b814526acb82def4b5810e394a2ed294f)

`QueryEngine` is a struct with one function pointer per query. This commit removes it by moving each function pointer into the relevant query's vtable, which is already a collection of function pointers for that query. This makes things simpler.

rust-timer · 2026-02-19T06:42:35Z

Finished benchmarking commit (6efacd9): comparison URL.

Overall result: ✅ improvements - no action needed

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.3%	[-0.5%, -0.1%]	10
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (secondary -3.2%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.2%	[-3.2%, -3.2%]	1
All ❌✅ (primary)	-	-	0

Cycles

Results (primary 2.1%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	2.1%	[2.1%, 2.1%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.1%	[2.1%, 2.1%]	1

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 487.064s -> 482.434s (-0.95%)
Artifact size: 397.85 MiB -> 397.80 MiB (-0.01%)

nnethercote · 2026-02-19T07:02:34Z

QueryEngine also merges into QueryVTable really nicely. I've added a new commit doing that.

I've been asking you for a lot of reviews; I'm happy to ask a different reviewer if you want a break.

rust-bors · 2026-02-20T09:48:24Z

☔ The latest upstream changes (presumably #152747) made this pull request unmergeable. Please resolve the merge conflicts.

Zalathar · 2026-02-20T10:36:25Z

I was skeptical of this PR at first, because it was unclear to me whether moving state/cache into the vtable was a good idea overall, and I wasn't convinced by the motivation of wanting to get rid of the unsafe offset fields (even though I don't like the offset fields).

However, after some more consideration, I'm coming around to the idea that putting per-query state/caches in the vtable is a fine idea in its own right. So now I'm pretty happy with the direction of this PR.

I have some remarks to write up, and I'll want to do another pass, but overall this PR looks good to me.

Zalathar · 2026-02-20T10:49:58Z

compiler/rustc_query_impl/src/plumbing.rs

+fn mk_query_stack_frame_extra<'tcx, K>(
+    (tcx, name, dep_kind, description_fn, key): (
+        TyCtxt<'tcx>,
+        &'static str,
+        DepKind,
+        fn(TyCtxt<'tcx>, K) -> String,
+        K,
+    ),


Suggestion: Based on my local testing, I think this entire commit can be avoided by just adding a QueryVTable<'tcx, C>: DynSync bound to create_deferred_query_stack_frame.

(This makes sense because the state and cache should be DynSync, and the caller knows that this happens to be true of every concrete state and cache.)

If you apply this suggestion, the commit description of the next commit will need some adjustment.

Zalathar · 2026-02-20T10:54:56Z

compiler/rustc_query_impl/src/plumbing.rs

+                    query_state: Default::default(),
+                    query_cache: Default::default(),


Question/suggestion: Should we get rid of the redundant query_ and rename these to state and cache?

Zalathar · 2026-02-20T10:59:01Z

compiler/rustc_middle/src/query/inner.rs

+pub(crate) fn query_get_at<'tcx, C>(
    tcx: TyCtxt<'tcx>,
-    execute_query: fn(TyCtxt<'tcx>, Span, Cache::Key, QueryMode) -> Option<Cache::Value>,
-    query_cache: &Cache,
+    execute_query: fn(TyCtxt<'tcx>, Span, C::Key, QueryMode) -> Option<C::Value>,
+    query_cache: &C,
    span: Span,
-    key: Cache::Key,
-) -> Cache::Value
+    key: C::Key,
+) -> C::Value
 where
-    Cache: QueryCache,
+    C: QueryCache,


Remark: I weakly prefer Cache over C, but if you feel strongly about it then I don't mind compromising on C. The single-letter convention does have the advantage of clearly indicating a type variable.

Zalathar · 2026-02-20T11:04:03Z

compiler/rustc_query_impl/src/plumbing.rs

-        }
-
-        pub fn make_query_vtables<'tcx>() -> queries::PerQueryVTables<'tcx> {
+        pub fn make_query_vtables<'tcx>(incremental: bool) -> queries::PerQueryVTables<'tcx> {


If you want to take this opportunity to also rename PerQueryVTables back to QueryVTables, feel free. The more verbose name did its job at a more confusing time, but the simpler QueryVTables should be fine now.

nnethercote added 3 commits February 19, 2026 14:18

rustbot assigned Zalathar Feb 19, 2026

This comment has been minimized.

Sign in to view

rust-bors bot pushed a commit that referenced this pull request Feb 19, 2026

Auto merge of #152835 - nnethercote:fix-query-state-query-cache, r=<try>

6efacd9

Improve how `QueryCache`s and `QueryState`s are stored

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 19, 2026

This comment has been minimized.

Sign in to view

Merge QueryEngine into QueryVTable.

bc792c2

`QueryEngine` is a struct with one function pointer per query. This commit removes it by moving each function pointer into the relevant query's vtable, which is already a collection of function pointers for that query. This makes things simpler.

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 19, 2026

Zalathar reviewed Feb 20, 2026

View reviewed changes

		query_state: Default::default(),
		query_cache: Default::default(),

Uh oh!

Comments

Conversation

nnethercote commented Feb 19, 2026

Uh oh!

rustbot commented Feb 19, 2026

Uh oh!

Zalathar commented Feb 19, 2026

Uh oh!

This comment has been minimized.

This comment has been minimized.

Zalathar commented Feb 19, 2026

Uh oh!

nnethercote commented Feb 19, 2026

Uh oh!

Zalathar commented Feb 19, 2026

Uh oh!

rust-bors bot commented Feb 19, 2026

Uh oh!

This comment has been minimized.

rust-timer commented Feb 19, 2026

Overall result: ✅ improvements - no action needed

Uh oh!

nnethercote commented Feb 19, 2026

Uh oh!

rust-bors bot commented Feb 20, 2026

Uh oh!

Zalathar commented Feb 20, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants